[WIP] New FastNoiseModelFactor with more efficient linearization #2275
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new FastNoiseModelFactorN class as an optimized alternative to the existing NoiseModelFactorN for more efficient linearization in GTSAM. The implementation aims to achieve approximately 10% performance improvement in linearization operations.
- Complete implementation of
FastNoiseModelFactorNwith optimized linearization usingVerticalBlockMatrix - Migration of
PriorFactorto use the new fast factor implementation - API adjustments to support Eigen::Ref-based Jacobian handling for better memory management
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| gtsam/nonlinear/FastNoiseModelFactorN.h | New optimized factor implementation with direct VerticalBlockMatrix linearization |
| gtsam/nonlinear/PriorFactor.h | Migration from NoiseModelFactorN to FastNoiseModelFactorN base class |
| gtsam/slam/tests/testPriorFactor.cpp | Updated test to use new evaluateError signature with nullptr parameter |
| gtsam/linear/NoiseModel.h | API changes to support Eigen::Block instead of Eigen::Block |
| gtsam/linear/JacobianFactor.h | Simplified constructor template for better move semantics |
| gtsam/linear/JacobianFactor-inl.h | Unified constructor implementation with perfect forwarding |
| gtsam/linear/GaussianConditional.cpp | Optimization to use std::move for VerticalBlockMatrix |
| gtsam/navigation/ImuBias.cpp | Added .eval() calls to fix potential Eigen expression template issues |
| gtsam/base/OptionalJacobian.h | New constructor overload for Eigen::Ref support |
|
Fan, I hope you saw that the CI fails. This could be due to unrelated changes you made. Maybe those should be a separate PR :-) i’m wondering whether it is necessary to have a completely new class or whether we can just have a different method to overload. Then we can deprecate the old method and publicize the new method. And this PR would also have much smaller change and be easier to review. |
ProfFan
force-pushed
the
fan/fast_nm_factor
branch
from
b6908be to
775ce42
Compare
ProfFan
force-pushed
the
fan/fast_nm_factor
branch
from
775ce42 to
374632c
Compare
|
@ProfFan Let's talk Monday about my comment and whether that's a good or a bad idea, and why... |
|
We should revive this so that we can be even faster |
About 10% faster in linearization with my microbenchmarks. I think there is still a large margin for optimization, but we should focus more on the linear solver...